Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #3258 - Handling Removal of the CircularImageView dependency #5350

Merged
merged 11 commits into from
Mar 20, 2024
Merged

Fix #3258 - Handling Removal of the CircularImageView dependency #5350

merged 11 commits into from
Mar 20, 2024

Conversation

Rd4dev
Copy link
Collaborator

@Rd4dev Rd4dev commented Mar 1, 2024 •
edited by adhiamboperes
Loading

Explanation

Fixes #3258

The Pull Request #4155 substituted CircularImageView with ShapeableImageView, yet the dependency persists in the project. This PR addresses the final steps of removing the remaining dependency.

add_shadow and shadow_radius were exclusive to CircularImageView. Upon removing the dependencies, these attributes became irrelevant, so they were omitted from the layouts. Their removal had no impact on the functioning views as they were unrelated to the actual attributes of the working views.
[Attached reference images below]

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

Having add_shadow

Todo

Remove Dependency from Bazel and External libraries.!

@Rd4dev Rd4dev requested review from a team as code owners March 1, 2024 21:36
@Rd4dev Rd4dev requested a review from BenHenning March 1, 2024 21:36
@Rd4dev Rd4dev mentioned this pull request Mar 1, 2024
Closed
@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Mar 1, 2024

@adhiamboperes PTAL

adhiamboperes
adhiamboperes reviewed Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Rd4dev!

Isn't there a declaration of this in third_party/versions.bzl?

@adhiamboperes adhiamboperes assigned Rd4dev and unassigned adhiamboperes Mar 8, 2024
@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Mar 11, 2024

Hi @adhiamboperes,

The third_party/versions.bzl file appears to be missing the CircularImageView dependency.
It seems that PR #2659 might have overlooked the addition of com.jackandphantom.android:circularimageview.

Two dependencies were initially employed for circular images:

Both dependencies were eventually mixed in layouts, with de.hdodenhof.circleimageview.CircleImageView predominantly
[ref. PR #4155].

Issue #3258 mentions replacing com.jackandphantom.android:circularimageview, but the comment suggests replacing de.hdodenhof.circleimageview.CircleImageView as well which was implemented in PR #4155.

Seeking clarification on whether it is appropriate to proceed with the removal of de.hdodenhof.circleimageview.CircleImageView, given that none of the layouts appear to utilize this dependency.

@adhiamboperes
Copy link
Collaborator

@Rd4dev , please remove the dependency as it is unused.

@Rd4dev Rd4dev requested review from a team as code owners March 14, 2024 17:54
@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Mar 14, 2024

@adhiamboperes PTAL

@oppiabot oppiabot bot assigned adhiamboperes and unassigned Rd4dev Mar 14, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 14, 2024

Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!

adhiamboperes
adhiamboperes requested changes Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Rd4dev, this looks good, but please resolve the merge conflict and then reassign to me.

@adhiamboperes adhiamboperes changed the title Fix part of #3258 - Handling Removal of the CircularImageView dependency Fix #3258 - Handling Removal of the CircularImageView dependency Mar 15, 2024
@Rd4dev
Copy link
Collaborator Author

Rd4dev commented Mar 15, 2024

@adhiamboperes PTAL

@oppiabot oppiabot bot assigned adhiamboperes and unassigned Rd4dev Mar 15, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 15, 2024

Unassigning @Rd4dev since a re-review was requested. @Rd4dev, please make sure you have addressed all review comments. Thanks!

adhiamboperes
adhiamboperes approved these changes Mar 18, 2024
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Rd4dev!

@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 18, 2024

Unassigning @adhiamboperes since they have already approved the PR.

@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 18, 2024

Assigning @BenHenning for code owner reviews. Thanks!

BenHenning
BenHenning approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice clean-up, thanks for doing this @Rd4dev! LGTM.

@BenHenning
Copy link
Member

Updating with latest & enabling auto-merge since everything seems to look good.

@BenHenning BenHenning enabled auto-merge (squash) March 19, 2024 23:48
@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 19, 2024

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Mar 19, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Mar 19, 2024

Hi @Rd4dev, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

@BenHenning BenHenning merged commit d0c8b81 into oppia:develop Mar 20, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adhiamboperes adhiamboperes adhiamboperes approved these changes

@BenHenning BenHenning BenHenning approved these changes

Assignees

@Rd4dev Rd4dev

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Find a maintained replacement for CircularImageView
3 participants
@Rd4dev @adhiamboperes @BenHenning